Repository navigation
Conversation
Map unrecognized InfoBarMessage names to the existing deprecated sentinel so they cannot throw through ApplicationState::FromJson during startup. Add regression coverage for mixed names and state serialization. Fixes microsoft#16874
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
|
@microsoft-github-policy-service agree |
|
Hi Dustin L. Howett (@DHowett)! Would you mind taking a look at this PR when you have a chance? |
There was a problem hiding this comment.
your fix is incomplete, as it doesn't remove these entries
|
|
||
| JSON_ENUM_MAPPER(::winrt::Microsoft::Terminal::Settings::Model::InfoBarMessage) | ||
| { | ||
| ValueType FromJson(const Json::Value& json) |
There was a problem hiding this comment.
Thanks for fixing the deserializer. What about ToJson? We should be able to prevent serializing IgnoredDeprecatedEntry altogether.
microsoft-github-policy-service
Bot
added
the
Needs-Author-Feedback
microsoft-github-policy-service
Bot
removed
the
Needs-Author-Feedback
Leonard Hecker (lhecker)
left a comment
Leonard Hecker (lhecker)
left a comment
There was a problem hiding this comment.
Otherwise, LGTM.
| // Invalid names must not prevent fields before or after this one from loading. | ||
| VERIFY_ARE_EQUAL(L"test-hash", state->SettingsHash()); | ||
| VERIFY_ARE_EQUAL(1u, state->AllowedCommandlines().Size()); | ||
| VERIFY_ARE_EQUAL(L"test-command", state->AllowedCommandlines().GetAt(0)); |
There was a problem hiding this comment.
Could you shrink the tests to just spot check the core issue to protect against future regressions?
(You got to really reign in your LLM when it comes to tests - they always tend to write overly defensive / broad tests. 😅)
There was a problem hiding this comment.
Haha, yeah 😅 I'll fix the tests and keep them focused on the bug. Thanks!
microsoft-github-policy-service
Bot
added
Needs-Author-Feedback
Summary of the Pull Request
Unknown dismissed-message names can throw through
ApplicationState::FromJsonand terminate startup. Treat them as ignored entries and omit them when saving. Remove the obsolete name mappings.Closes #16874.
Validation
One regression test loads an unknown name alongside
closeOnExitInfo, then checks that only the known dismissal is saved.Prepared with AI assistance.